-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLI: Various improvements concerning user details #6173
Conversation
This is a useful shortcut to determine whether a `User` instance is the current default user. The previous way of determing this was to retrieve the default user from the collection `User.collection.get_default()` and manually compare it with the `User` instance.
Uses the `tabulate` package to create a nicely formatted table as is used in many other `verdi` commands already. The results are ordered by the users emails.
Each profile can define which user in its storage backend should be considered the default. This is necessary because each ORM entity, when created, needs to specify a `User` object and we don't want the user to always have to explicitly define this manuallly. The default user for a profile is stored in the configuration file by the email of the `User` object. However, in a loaded storage backend, this default user is also cached as the `User` object. This means that when the default user is changed, it should be changed both in the configuration file, but if a storage backend is loaded, the cache should also be invalidated, such that the next time the default user is requested, the new one is properly loaded from the database. Since this change affects both the configuration as well as the currently loaded storage, the `set_default_user_email` is added to the `Manager` class, since that controls both. It calls through to the same method on the `Config` class, which is responsible for updating the `Config` instance in memory and writing the changes to disk. Then the manager resets the default user on the storage backend, if any is loaded. The `verdi user set-default` command is updated to use the new method. A test is added for the command, which didn't exist yet. The command is updated to use `Manager.set_default_user_email` even though it could use `Config.set_default_user_email` since the Python interpreter will shut down immediately after anyway. However, the test would fail if the latter would be used, since the loaded storage backend would not have been updated, which is used by `User.collection.get_default()`. This demonstrates why in active Python interpreters only the method on the manager should be used. A warning is added to the docstring on the configuration class.
1e65e99
to
99f8609
Compare
For sure, thanks @sphuber, always happy to help lower the barrier for users to get started with AiiDA. ^^ The message of that final commit is music to my ears. Quick first question (which I probably asked before): why can the email not be changed later on? EDIT: Ah, I see, is the email sort of used as an identifier, since: Lines 144 to 149 in 1d8ea2a
|
Exactly, the In [1]: u = User.collection.get(id=1)
Out[1]: <aiida.orm.users.User at 0x7fd6ad348a90>
In [2]: u.email = 'test@localhost' So we might want to consider making the email immutable. But maybe it would be even better to keep it mutable and simply add a proper UUID to the There are also potential problems lying in wait with archives: what if you want to import an archive that happens to have a user with the same email? Currently you can't. Seems a bit annoying though and I am suprised it hasn't actually cropped up before. Or maybe the import mechanism has a way of dealing with it and automatically changes the email for the imported user. And what happens if you import a user with the same email that actually represents the same user, but the name and institution have changed. Which version do you keep? Anyway, it is clear that this requires some more thought, but that is for a later time. |
Thanks for the explanation @sphuber!
Hehe, yeah, even I wouldn't dare to introduce such scope creep in a PR. ;) So I won't derail the discussion here, but do we have an open issue about this topic? Or a discussion somewhere? I couldn't immediately find one. |
No, not that I recall. I will open one now |
highlight = lambda x: x.email == default_user.email if default_user else None | ||
echo.echo_formatted_list(User.collection.all(), attributes, sort=sort, highlight=highlight) | ||
if User.collection.get_default() is None: | ||
echo.echo_warning('No default user has been configured') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if this ever can happen. One needs to set up a user to set up a profile, and that would immediately set this user as the default. If you haven't set up a profile, you get:
❯ verdi user list
Critical: no default profile defined: None
{'CONFIG_VERSION': {'CURRENT': 9, 'OLDEST_COMPATIBLE': 9}, 'profiles': {}, 'options': {'autofill.user.email': 'idontwantto@tell.you', 'autofill.user.first_name': 'John', 'autofill.user.last_name': 'Doe', 'autofill.user.institution': 'Unknown'}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is possible. If you delete the user, or you simply change the user's email, or even if the config is corrupted and the default_user_email
key is removed. Lot's of possibilities :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, I guess you're right
❯ verdi user list
Email First name Last name Institution
-- --------- ------------ ----------- -------------
mwhahahha John Doe Unknown
Warning: No default user has been configured
But these paths are probably not really desirable, since once you don't have a default user, you can't even store a simple Int
node.
Side tangent: I checked out the docstring of the store method, and it thoroughly confused me.
Utter confusion
In [4]: Int(1).store?
Docstring:
Lightweight persistence for python variables.
Example::
In [1]: l = ['hello',10,'world']
In [2]: %store l
Stored 'l' (list)
In [3]: exit
(IPython session is closed and started again...)
ville@badger:~$ ipython
In [1]: l
NameError: name 'l' is not defined
In [2]: %store -r
In [3]: l
Out[3]: ['hello', 10, 'world']
Usage:
* ``%store`` - Show list of all variables and their current
values
* ``%store spam bar`` - Store the *current* value of the variables spam
and bar to disk
* ``%store -d spam`` - Remove the variable and its value from storage
* ``%store -z`` - Remove all variables from storage
* ``%store -r`` - Refresh all variables, aliases and directory history
from store (overwrite current vals)
* ``%store -r spam bar`` - Refresh specified variables and aliases from store
(delete current val)
* ``%store foo >a.txt`` - Store value of foo to new file a.txt
* ``%store foo >>a.txt`` - Append value of foo to file a.txt
It should be noted that if you change the value of a variable, you
need to %store it again if you want to persist the new value.
Note also that the variables will need to be pickleable; most basic
python types can be safely %store'd.
Also aliases can be %store'd across sessions.
To remove an alias from the storage, use the %unalias magic.
File: ~/.virtualenvs/tmp/lib/python3.11/site-packages/IPython/extensions/storemagic.py
This way it is guaranteed that the same types are being used, which were actually different. The `--set-default` flag now also gets its default from the current value on the selected user, just as is done for the other properties.
The user options `--first-name`, `--last-name` and `--institution` in the `verdi quicksetup/setup` commands were recently made required but did not provide a default. This would make creating profiles significantly more complex than always needed. For simple test and demo profiles the user might not necessarily care about these user details. Here we add defaults for these options. Even for production profiles this is a sensible approach since these details can always be freely updated later on with `verdi user configure`. This is also the reason that the `--email` does not provide a default because that can not be changed later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sphuber ok, I reviewed the commits one by one and have no further comments. So this one is good to go for me! 🚀
99f8609
to
2bb6128
Compare
Thanks a lot for the review @mbercx |
This PR consists of a number of commits that are purposefully kept separate. For review, it would make most sense to look at those commits one by one. If this is too complicated, I am willing to split it up in separate PRs but since they all touch the same files I kept it together for now.
The last commit is what is I really need for #6148
We recently fixed a bug in the
verdi quicksetup/setup
command where it was possible to not provide anything for the users first name, last name and institution. They are now properly required, but that poses a problem for the UX for the newSqliteDosStorage
. The idea is that creating such a profile is dead easy, but due to this change, the user is now forced to specify these options, where often we don't really care.The last commit adds a default for these options, solving the problem. The other commits are various improvements to the
verdi user
commands and an actual bug fix when changing default users in a persistent Python interpreter.@mbercx do you want to take this one perhaps since you have been an advocate of not requiring the user details on setup?